Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alchemical ion improvements #52

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Alchemical ion improvements #52

merged 4 commits into from
Aug 9, 2024

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Aug 9, 2024

  1. Automatically keep the charge constant unless the user has specified a value, warning if value doesn't match.
  2. If present, use existing ion parameters when creating the alchemical ion.

(I'm just finding a system to test this on locally.)

@lohedges lohedges added enhancement New feature or request cresset Related to work with Cresset labels Aug 9, 2024
@lohedges lohedges requested a review from mb2055 August 9, 2024 12:02
@mb2055
Copy link
Contributor

mb2055 commented Aug 9, 2024

I don't think this automatically keeps the charge neutral. If i pass a system with a charge difference of 1 without specifying --charge-difference I simply get the warning WARNING | somd2.runner._runner:__init__:187 - The charge difference of 1 between the end states does not match the specified value of 0..
Should it not automatically detect that there is a difference of 1 and create a corresponding co-alchemical ion?

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

That should just be a warning that it doesn't match the specified value. Since the user hasn't set anything in this case (the default is zero) it should still create the ion. Do you not see anything else in the log, e.g. Water at molecule index.... I'll have a check too.

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

No, you're right, it's not automatically updating. Will debug.

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

I was using the wrong variable for the charge difference in one of the conditionals. Should hopefully work now. For example, if the actual charge difference is 1 but I specify 3, then I see:

2024-08-09 15:41:59.202 | WARNING  | somd2.runner._runner:__init__:181 - Charge difference between end states is not an integer.
2024-08-09 15:41:59.203 | WARNING  | somd2.runner._runner:__init__:187 - The charge difference of 1 between the end states does not match the specified value of 3
2024-08-09 15:41:59.211 | INFO     | somd2.runner._runner:_create_alchemical_ions:450 - Water at molecule index 489 will be perturbed to Cl- to keep charge constant.
2024-08-09 15:41:59.215 | INFO     | somd2.runner._runner:_create_alchemical_ions:450 - Water at molecule index 101 will be perturbed to Cl- to keep charge constant.
2024-08-09 15:41:59.219 | INFO     | somd2.runner._runner:_create_alchemical_ions:450 - Water at molecule index 861 will be perturbed to Cl- to keep charge constant.

But if I don't specify anything I get:

2024-08-09 15:43:39.006 | WARNING  | somd2.runner._runner:__init__:181 - Charge difference between end states is not an integer.
2024-08-09 15:43:39.006 | WARNING  | somd2.runner._runner:__init__:187 - The charge difference of 1 between the end states does not match the specified value of 0
2024-08-09 15:43:39.016 | INFO     | somd2.runner._runner:_create_alchemical_ions:450 - Water at molecule index 489 will be perturbed to Cl- to keep charge constant.

@mb2055
Copy link
Contributor

mb2055 commented Aug 9, 2024

Yes, it works now.
The only issue I foresee is inconsistency when charge-difference is specified with a value of 0.
Currently if I have a system with a non-zero charge difference and I specify somd2 Input.bss --pert-file Input.pert --charge-difference 0 the code will automatically introduce an alchemical ion:

2024-08-09 15:44:09.323 | WARNING  | somd2.runner._runner:__init__:181 - Charge difference between end states is not an integer.
2024-08-09 15:44:09.323 | WARNING  | somd2.runner._runner:__init__:187 - The charge difference of 1 between the end states does not match the specified value of 0
2024-08-09 15:44:09.332 | INFO     | somd2.runner._runner:_create_alchemical_ions:450 - Water at molecule index 489 will be perturbed to Cl- to keep charge constant.

This, as far as I can tell, makes it impossible to run without an alchemical ion in systems with a charge difference, which could be an issue.

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

Ah yes, I need to think of the best way of checking that the value isn't default. And the user has explicitly set it to zero. This might be tricky, since I can imagine input coming from the command-line, YAML file, etc. We'd need to make sure not to write a default value to the YAML, since if it was re-read then it would appear as having been set, etc.

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

I'll also add some checks for sensible numbers for the charge difference, e.g. so that it doesn't exceed the number of water molecules. (Not that they should be doing that anyway.)

@lohedges
Copy link
Contributor Author

lohedges commented Aug 9, 2024

Just defaulting to None for the charge_difference should fix things. I've also added a check for the number of water molecules.

Copy link
Contributor

@mb2055 mb2055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good 👍

@lohedges lohedges merged commit ac23beb into main Aug 9, 2024
4 of 6 checks passed
@lohedges lohedges deleted the feature_alchemical_ion2 branch August 9, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cresset Related to work with Cresset enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants